Skip to content

C#: avoid calls to Location::toString() #13361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 6, 2023
Merged

C#: avoid calls to Location::toString() #13361

merged 2 commits into from
Jun 6, 2023

Conversation

nickrolfe
Copy link
Contributor

Before

Most expensive predicates for completed query ComplexCondition.ql:
        time  | evals |   max @ iter | predicate
        ------|-------|--------------|----------
        22.5s |       |              | Location#a178a2aa::Location::toString#0#dispred#ff@736e93jb
         4.2s |       |              | locations_default_234501#join_rhs@ad8924cn
           2s |       |              | boundedFastTC:ExprOrStmtParent#051be664::Cached::parent#2#ff_10#higher_order_body:project#Callable#f85cebf6::Callable::getBody#0#dispred#ff@37a64ask
           2s |       |              | boundedFastTC:ExprOrStmtParent#051be664::Cached::parent#2#ff_10#higher_order_body:Callable#f85cebf6::Constructor::getInitializer#0#dispred#ff_1#higher_order_body@6cf8660h

It's not just slow, it also results in a lot of string-pool insertions:

Evaluated non-recursive predicate Location#a178a2aa::Location::toString#0#dispred#ff@736e93jb in 22467ms (size: 14941648).
Evaluated relational algebra for predicate Location#a178a2aa::Location::toString#0#dispred#ff@736e93jb with tuple counts:
             702   ~0%    {2} r1 = SCAN assemblies OUTPUT In.0, In.2
                      
        14940946   ~0%    {2} r2 = SCAN Location#a178a2aa::SourceLocation::hasLocationInfo#5#dispred#ffffff OUTPUT In.0, (In.1 ++ ":" ++ toString(In.2) ++ ":" ++ toString(In.3) ++ ":" ++ toString(In.4) ++ ":" ++ toString(In.5))
                      
        14941648   ~0%    {2} r3 = r1 UNION r2
                          return r3

After

Most expensive predicates for completed query ComplexCondition.ql:
        time  | evals |   max @ iter | predicate
        ------|-------|--------------|----------
         9.5s |       |              | Element#5e6df1e8::Element::toString#0#dispred#ff@fcfdfe7e
         4.8s |    22 |  1.3s @ 9    | Element#5e6df1e8::NamedElement::getQualifiedName#0#dispred#ff@0ab92y65
         3.8s |       |              | locations_default_234501#join_rhs@ad8924cn
         3.4s |    22 | 227ms @ 6    | Generics#b6772a26::getTypeArgumentsQualifiedNames#1#ff@0ab92w65

It looks like this still results in an expensive call to Element::toString(), which is not ideal, but I think it's still a slight improvement.

Test?

There doesn't appear to be a test for this query.

@nickrolfe nickrolfe added C# no-change-note-required This PR does not need a change note labels Jun 2, 2023
@nickrolfe nickrolfe requested a review from a team as a code owner June 2, 2023 18:43
@nickrolfe
Copy link
Contributor Author

I've pushed a commit for another one I found, similar to #13348 for Java.

Before

Most expensive predicates for completed query ExposeRepresentation.ql:
        time  | evals  |   max @ iter | predicate
        ------|--------|--------------|----------
        26.1s |        |              | Location#a178a2aa::Location::toString#0#dispred#ff@736e93jb
        16.9s |        |              | Element#5e6df1e8::Element::toString#0#dispred#ff@fcfdfe7e
        11.8s |        |              | ControlFlow#55a48070::ControlFlowNode::getASuccessorType#1#dispred#fff@1f60c14h

After

Most expensive predicates for completed query ExposeRepresentation.ql:
        time  | evals  |   max @ iter | predicate
        ------|--------|--------------|----------
        14.9s |        |              | Element#5e6df1e8::Element::toString#0#dispred#ff@fcfdfe7e
        11.4s |        |              | ControlFlow#55a48070::ControlFlowNode::getASuccessorType#1#dispred#fff@1f60c14h
         9.9s |        |              | boundedFastTC:cil_method_source_declaration_10#higher_order_body:cil_method_location_0#higher_order_body@f60024un

@nickrolfe nickrolfe changed the title C#: avoid call to Location::toString() in cs/complex-condition C#: avoid calls to Location::toString() Jun 5, 2023
@nickrolfe nickrolfe mentioned this pull request Jun 5, 2023
Merged
hvitved
hvitved approved these changes Jun 6, 2023
View reviewed changes
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@nickrolfe nickrolfe merged commit 3d0ecbe into main Jun 6, 2023
@nickrolfe nickrolfe deleted the nickrolfe/csharp-location-tostring branch June 6, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hvitved hvitved hvitved approved these changes

Assignees
No one assigned
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nickrolfe @hvitved